Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reduce TextFormatter allocations #3991

Merged
merged 12 commits into from
Mar 17, 2025
Merged

Reduce TextFormatter allocations #3991

merged 12 commits into from
Mar 17, 2025

Conversation

TheTonttu
Copy link
Contributor

Reduces intermediate allocations in the simplest top of the chart TextFormatter helper methods, and StringExtensions.ToString(IEnumerable<Rune>), which is used everywhere.

Fixes

Proposed Changes

  • Rewrite various TextFormatter helper methods to reduce allocations.
    • RemoveHotKeySpecifier
      • The usual char stackalloc char buffer with rented array as alternative.
    • ReplaceCRLFWithSpace
      • StringBuilder append with early original string return if no newlines found.
    • StripCRLF
      • Almost identical to ReplaceCRLFWithSpace.
  • Rewrite StringExtensions.ToString(IEnumerable<Rune>) to reduce allocations.
    • Again the usual stackalloc char buffer with rented array as alternative. StringBuilder is used as fallback if element count is not available without enumeration.
  • Change ReplaceCRLFWithSpace and StripCRLF accessibility level from private to internal.
  • Add test cases for ReplaceCRLFWithSpace and StripCRLF.

Pull Request Checklist

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

Microbenchmarks

StringExtensions.ToString(IEnumerable<Rune>)

Note that this only benchmarks the non-StringBuilder-fallback side. Most of the call sites use Rune[] or List<Rune> anyways so the fallback is not that relevant. Might add benchmarks for the fallback later.

The input lengths are a bit overkill but you get the idea.


BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5608/22H2/2022Update)
AMD Ryzen 7 5800X3D, 1 CPU, 16 logical and 8 physical cores
.NET SDK 8.0.407
  [Host]     : .NET 8.0.14 (8.0.1425.11118), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.14 (8.0.1425.11118), X64 RyuJIT AVX2
Method runes len Mean Error StdDev Ratio RatioSD Gen0 Gen1 Allocated Alloc Ratio
Previous Rune[1] 1 10.24 ns 0.247 ns 0.469 ns 0.39 0.02 0.0011 - 56 B 1.00
Current Rune[1] 1 25.97 ns 0.138 ns 0.116 ns 1.00 0.01 0.0011 - 56 B 1.00
Previous Rune[10] 10 118.36 ns 0.350 ns 0.273 ns 2.01 0.02 0.0119 - 608 B 7.60
Current Rune[10] 10 58.89 ns 0.577 ns 0.511 ns 1.00 0.01 0.0015 - 80 B 1.00
Previous Rune[100] 100 1,415.36 ns 19.094 ns 17.861 ns 3.62 0.08 0.2975 - 15008 B 58.62
Current Rune[100] 100 391.48 ns 7.686 ns 7.190 ns 1.00 0.03 0.0048 - 256 B 1.00
Previous Rune[401] 401 9,011.12 ns 51.757 ns 45.881 ns 6.20 0.03 3.6011 - 180856 B 211.28
Current Rune[401] 401 1,453.04 ns 2.656 ns 2.073 ns 1.00 0.00 0.0153 - 856 B 1.00
Previous Rune[802] 802 27,098.45 ns 175.132 ns 163.819 ns 9.43 0.09 13.6108 0.0305 683312 B 410.64
Current Rune[802] 802 2,873.37 ns 23.531 ns 20.860 ns 1.00 0.01 0.0305 - 1664 B 1.00
Previous Rune[8020] 8020 2,042,258.31 ns 19,643.001 ns 17,413.008 ns 72.38 0.61 1289.0625 50.7813 64721410 B 4,020.96
Current Rune[8020] 8020 28,215.83 ns 52.173 ns 43.567 ns 1.00 0.00 0.3052 - 16096 B 1.00

TextFormatter.RemoveHotKeySpecifier


BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5608/22H2/2022Update)
AMD Ryzen 7 5800X3D, 1 CPU, 16 logical and 8 physical cores
.NET SDK 8.0.407
  [Host]     : .NET 8.0.14 (8.0.1425.11118), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.14 (8.0.1425.11118), X64 RyuJIT AVX2
Method text hotPos Mean Error StdDev Ratio RatioSD Gen0 Allocated Alloc Ratio
Previous **** -1 0.9392 ns 0.0499 ns 0.0466 ns 0.54 0.03 - - NA
Current -1 1.7463 ns 0.0687 ns 0.0643 ns 1.00 0.05 - - NA
Previous _Save file (Ctrl+S) 0 218.4687 ns 2.5621 ns 2.2713 ns 3.19 0.05 0.0238 1200 B 18.75
Current _Save file (Ctrl+S) 0 68.3901 ns 0.8073 ns 0.7551 ns 1.00 0.02 0.0012 64 B 1.00
Previous Lore(...)lla. [155] 82 2,525.6022 ns 12.5621 ns 10.4899 ns 5.53 0.02 0.6218 31392 B 93.43
Current Lore(...)lla. [155] 82 456.4663 ns 0.7380 ns 0.5762 ns 1.00 0.00 0.0067 336 B 1.00
Previous Ĺόŕé(...) íń. [471] 64 11,531.7165 ns 198.1963 ns 175.6959 ns 7.91 0.12 4.8676 244376 B 252.45
Current Ĺόŕé(...) íń. [471] 64 1,458.4510 ns 3.5958 ns 3.1876 ns 1.00 0.00 0.0191 968 B 1.00
Previous Ĺόŕé(...) íń. [471] 409 11,517.6575 ns 210.3947 ns 196.8033 ns 7.86 0.15 4.8676 244376 B 252.45
Current Ĺόŕé(...) íń. [471] 409 1,466.2623 ns 17.0662 ns 15.1287 ns 1.00 0.01 0.0191 968 B 1.00
Previous Save file (Ctrl+S) 3 220.2640 ns 3.0005 ns 2.8067 ns 4.00 0.05 0.0238 1200 B NA
Current Save file (Ctrl+S) 3 55.0176 ns 0.1739 ns 0.1358 ns 1.00 0.00 - - NA

TextFormatter.ReplaceCRLFWithSpace


BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5608/22H2/2022Update)
AMD Ryzen 7 5800X3D, 1 CPU, 16 logical and 8 physical cores
.NET SDK 8.0.407
  [Host]     : .NET 8.0.14 (8.0.1425.11118), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.14 (8.0.1425.11118), X64 RyuJIT AVX2
Method str Mean Error StdDev Ratio RatioSD Gen0 Allocated Alloc Ratio
Previous E\r\nx\r(...)\r\no\r\n [66] 745.67 ns 4.089 ns 3.625 ns 3.62 0.02 0.0277 1400 B 3.07
Current E\r\nx\r(...)\r\no\r\n [66] 206.13 ns 0.823 ns 0.730 ns 1.00 0.00 0.0091 456 B 1.00
Previous Lore(...) eu. [698] 5,651.54 ns 30.978 ns 24.185 ns 462.17 3.45 0.1984 9952 B NA
Current Lore(...) eu. [698] 12.23 ns 0.095 ns 0.079 ns 1.00 0.01 - - NA
Previous Ĺόŕé(...)ĺíś. [806] 6,662.39 ns 42.272 ns 35.299 ns 28.36 0.20 0.1984 10160 B 2.22
Current Ĺόŕé(...)ĺíś. [806] 234.91 ns 1.447 ns 1.208 ns 1.00 0.01 0.0913 4584 B 1.00

TextFormatter.StripCRLF


BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5608/22H2/2022Update)
AMD Ryzen 7 5800X3D, 1 CPU, 16 logical and 8 physical cores
.NET SDK 8.0.407
  [Host]     : .NET 8.0.14 (8.0.1425.11118), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.14 (8.0.1425.11118), X64 RyuJIT AVX2
Method str keepNewLine Mean Error StdDev Ratio RatioSD Gen0 Gen1 Allocated Alloc Ratio
Previous E\r\nx\r(...)\r\no\r\n [66] False 844.14 ns 4.025 ns 3.361 ns 4.70 0.05 0.0267 - 1376 B 4.91
Current E\r\nx\r(...)\r\no\r\n [66] False 179.70 ns 2.091 ns 1.956 ns 1.00 0.01 0.0055 - 280 B 1.00
Previous E\r\nx\r(...)\r\no\r\n [66] True 742.83 ns 1.616 ns 1.262 ns 3.57 0.02 0.0277 - 1400 B 3.07
Current E\r\nx\r(...)\r\no\r\n [66] True 207.93 ns 1.654 ns 1.381 ns 1.00 0.01 0.0091 - 456 B 1.00
Previous Lore(...) eu. [698] False 5,746.73 ns 64.305 ns 57.005 ns 425.75 4.13 0.1984 - 9952 B NA
Current Lore(...) eu. [698] False 13.50 ns 0.028 ns 0.022 ns 1.00 0.00 - - - NA
Previous Lore(...) eu. [698] True 5,511.28 ns 14.646 ns 12.230 ns 403.05 7.69 0.1984 - 9952 B NA
Current Lore(...) eu. [698] True 13.68 ns 0.291 ns 0.272 ns 1.00 0.03 - - - NA
Previous Ĺόŕé(...)ĺíś. [806] False 6,463.90 ns 38.463 ns 30.030 ns 28.00 0.31 0.1984 - 10152 B 2.18
Current Ĺόŕé(...)ĺíś. [806] False 230.89 ns 2.605 ns 2.436 ns 1.00 0.01 0.0925 0.0005 4648 B 1.00
Previous Ĺόŕé(...)ĺíś. [806] True 6,667.01 ns 34.121 ns 26.640 ns 28.22 0.12 0.1984 - 10160 B 2.22
Current Ĺόŕé(...)ĺíś. [806] True 236.23 ns 0.579 ns 0.452 ns 1.00 0.00 0.0913 - 4584 B 1.00

Allocations

Overall nice reduction in string allocations. Naturally StringBuilder (and its internal char[]) allocations increased a bit because they are now used in ReplaceCRLFWithSpace and StripCRLF.

Before After
01 string-allocations before 02 string-allocations after
01-01 drilldown-string-concat before 02-01 drilldown-string-concat after
01-02 drilldown-string-extensions before 02-02 drilldown-string-extensions after
01-03 string-builder-allocations before 02-03 string-builder-allocations after
01-04 drilldown-string-builder before 02-04 drilldown-string-builder after
01-05 drilldown-char-array before 02-05 drilldown-char-array after

TheTonttu added 12 commits March 16, 2025 10:06
@TheTonttu
Add tests for TextFormatter StripCRLF and ReplaceCRLFWithSpace
51ebf03 
Also made the helper methods internal so they can be accessed from the test project.
@TheTonttu
Rewrite TextFormatter.StripCRLF
a741904 
Uses StringBuilder and char span indexof search to reduce intermediate allocations.

The new implementation behaves slightly different compared to old implementation. In synthetic LFCR scenario it is correctly removed while the old implementation left the CR, which seems like an off-by-one error.
@TheTonttu
More varied text permutations for the StripCRLF benchmark
4dc4776
@TheTonttu
StripCRLF early exit when no newline to avoid StringBuilder allocation
f5ccaa4
@TheTonttu
Rewrite TextFormatter.ReplaceCRLFWithSpace
658160d 
Almost identical to the StripCRLF implementation.
@TheTonttu
Add benchmark category to the TextFormatter benchmarks
cb25522
@TheTonttu
Rewrite StringExtensions.ToString(IEnumerable<Rune>)
924dc11 
Appends rune chars to StringBuilder avoiding intermediate string allocation for each rune append.
@TheTonttu
StringExtensions.ToString(IEnumerable<Rune>) stackalloc char buffer w…
196169d 
…ith StringBuilder fallback
@TheTonttu
Rewrite TextFormatter.RemoveHotKeySpecifier
77a0296 
Uses stackalloc char buffer with fallback to rented array.
@TheTonttu
StringExtensions.ToString(IEnumerable<Rune>) remove extra rune chars …
e88351c 
…copy
@TheTonttu
StringExtensions.ToString(IEnumerable<Rune>) use rented array as alte…
076b2ca 
…rnative buffer
@TheTonttu
Consistent kilobyte unit in comments
601bc97
@TheTonttu TheTonttu requested a review from tig as a code owner March 16, 2025 10:18
TheTonttu
TheTonttu commented Mar 16, 2025
View reviewed changes
Terminal.Gui/Text/TextFormatter.cs
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently document reformat caused a bunch of indentation changes. I recommend ignoring whitespace.

tig
tig approved these changes Mar 17, 2025
View reviewed changes
Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noble and excellent work. Thank you!

FWIW, see #3617

@tig tig merged commit 9735d8c into gui-cs:v2_develop Mar 17, 2025
11 checks passed
@TheTonttu
Copy link
Contributor Author

FWIW, see #3617

Yeah, this was very much a bandage optimization. 😄

@TheTonttu TheTonttu deleted the reduce-text-formatter-allocations branch March 17, 2025 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tig tig tig approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TheTonttu @tig